Skip to content

Optimize dev rebuild defaults - #86

Open
ScriptedAlchemy wants to merge 15 commits into
mainfrom
codex/dev-rebuild-optimizations-20260708
Open

Optimize dev rebuild defaults#86
ScriptedAlchemy wants to merge 15 commits into
mainfrom
codex/dev-rebuild-optimizations-20260708

Conversation

@ScriptedAlchemy

Copy link
Copy Markdown
Collaborator

Summary

  • stop forcing dev writeToDisk so Rsbuild keeps dev output in its memory filesystem
  • disable classic web splitChunks/runtimeChunk in dev by default while preserving user rspack optimization overrides
  • cache identical route transform results across inline/worker executors

Verification

  • pnpm test:core
  • pnpm build
  • pnpm bench:smoke
  • node scripts/bench-builds.mts --profile smoke --mode dev --iterations 1 --warmup 0 --format both --out .benchmark/results/dev-smoke
  • pnpm prettier --check src/index.ts src/parallel-route-transforms.ts tests/index.test.ts tests/features.test.ts tests/parallel-route-transforms.test.ts tests/dev-runtime.integration.test.ts
  • git diff --check

@pkg-pr-new

pkg-pr-new Bot commented Jul 8, 2026

Copy link
Copy Markdown

Open in StackBlitz

pnpm add https://pkg.pr.new/rsbuild-plugin-react-router@29912bf

commit: 29912bf

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Benchmark Results

Compared PR head 29912bf against base 94ed3cc.

Reading benchmark confidence

Raw deltas are always shown. The signal label only indicates whether the observed median delta is larger than a robust run-to-run noise band; it does not erase or replace the measurement.

The noise band is the larger of 2% or two combined robust standard deviations estimated from each side's relative median absolute deviation (rMAD). Fewer than three finite samples is reported as insufficient data. An inconclusive result should be rerun or investigated from the uploaded raw samples before drawing a performance conclusion.

Benchmark Runs Base total Head total Delta Base rMAD Head rMAD Noise band Signal
large-355-ssr-esm (build) 3 8.92s 8.74s -2.0% 0.8% 0.3% ±2.5% ⚪ inconclusive
synthetic-1024-ssr-esm (build) 3 4.28s 4.38s +2.4% 1.5% 1.2% ±5.6% ⚪ inconclusive
synthetic-1024-ssr-esm-split (build) 3 5.71s 5.82s +1.9% 0.8% 0.9% ±3.5% ⚪ inconclusive
synthetic-256-sourcemaps (build) 5 2.13s 2.10s -1.4% 0.2% 0.2% ±2.0% ⚪ inconclusive
synthetic-256-ssr-esm (build) 5 2.01s 1.95s -2.8% 0.2% 0.9% ±2.6% 🟢 improvement
synthetic-256-ssr-esm-split (build) 5 2.37s 2.28s -3.6% 1.6% 0.0% ±4.6% ⚪ inconclusive
synthetic-48-ssr-esm (build) 5 1.32s 1.32s +0.2% 0.4% 2.1% ±6.2% ⚪ inconclusive
large-355-ssr-esm (dev) 3 14.66s 14.71s +0.3% 0.6% 0.1% ±2.0% ⚪ inconclusive
synthetic-1024-ssr-esm (dev) 3 4.67s 4.62s -1.1% 0.3% 0.8% ±2.5% ⚪ inconclusive
synthetic-1024-ssr-esm-split (dev) 3 4.66s 4.60s -1.4% 1.5% 1.7% ±6.7% ⚪ inconclusive
synthetic-256-sourcemaps (dev) 5 1.84s 1.80s -1.8% 0.5% 0.7% ±2.5% ⚪ inconclusive
synthetic-256-ssr-esm (dev) 5 1.68s 1.67s -0.5% 1.3% 1.1% ±5.1% ⚪ inconclusive
synthetic-256-ssr-esm-split (dev) 5 1.70s 1.66s -2.4% 0.4% 1.5% ±4.5% ⚪ inconclusive
synthetic-48-ssr-esm (dev) 5 0.89s 0.87s -2.6% 1.8% 1.4% ±6.9% ⚪ inconclusive
complex app (cold) 3 111.61s 118.72s +6.4% 0.4% 0.6% ±2.1% 🔴 regression
complex app (dev) 3 102.23s 103.60s +1.3% 0.3% 0.3% ±2.0% ⚪ inconclusive
complex app (warm) 3 83.64s 84.45s +1.0% 0.4% 0.9% ±2.9% ⚪ inconclusive

Dev Rollup

Group Fixtures Base total Head total Delta Base ready Head ready Ready delta Base routes Head routes Route delta Base update/HMR Head update/HMR Update delta Speedup
All dev fixtures 7 30.11s 29.93s -0.6% 18.79s 18.77s -0.1% 4.04s 3.95s -2.2% 2.63s 2.68s +1.8% 1.01x
Large app 1 14.66s 14.71s +0.3% 8.04s 8.06s +0.3% 1.98s 2.02s +1.9% 1.57s 1.57s +0.0% 1.00x
Standard fixtures 6 15.44s 15.22s -1.4% 10.75s 10.71s -0.4% 2.05s 1.93s -6.2% 1.06s 1.11s +4.3% 1.01x

Production Build Benchmarks

Rendered 7 production build benchmarks.

Benchmark Runs Base total Head total Delta Head mean Head p95 Speedup Head RSS p95
large-355-ssr-esm 3 8.92s 8.74s -2.0% 8.77s 8.84s 1.02x 1435 MB
synthetic-1024-ssr-esm 3 4.28s 4.38s +2.4% 4.40s 4.50s 0.98x 550 MB
synthetic-1024-ssr-esm-split 3 5.71s 5.82s +1.9% 5.78s 5.87s 0.98x 643 MB
synthetic-256-sourcemaps 5 2.13s 2.10s -1.4% 2.10s 2.11s 1.01x 401 MB
synthetic-256-ssr-esm 5 2.01s 1.95s -2.8% 1.95s 1.97s 1.03x 347 MB
synthetic-256-ssr-esm-split 5 2.37s 2.28s -3.6% 2.29s 2.33s 1.04x 371 MB
synthetic-48-ssr-esm 5 1.32s 1.32s +0.2% 1.32s 1.36s 1.00x 295 MB

ci-small+ci-large Dev Fixture Summary

Rendered 7 dev benchmark fixtures from the ci-small+ci-large profile.

Benchmark Runs Base total Head total Delta Base ready Head ready Base routes Head routes Base update/HMR Head update/HMR Update delta Head mean Head p95 Speedup Head RSS p95
large-355-ssr-esm 3 14.66s 14.71s +0.3% 8.04s 8.06s 1.98s 2.02s 1.57s 1.57s +0.0% 14.71s 14.73s 1.00x -
synthetic-1024-ssr-esm 3 4.67s 4.62s -1.1% 3.27s 3.26s 0.59s 0.55s 0.35s 0.35s +0.1% 4.61s 4.66s 1.01x -
synthetic-1024-ssr-esm-split 3 4.66s 4.60s -1.4% 3.19s 3.24s 0.61s 0.55s 0.33s 0.35s +7.2% 4.61s 4.70s 1.01x -
synthetic-256-sourcemaps 5 1.84s 1.80s -1.8% 1.32s 1.30s 0.24s 0.22s 0.13s 0.13s +0.8% 1.80s 1.82s 1.02x -
synthetic-256-ssr-esm 5 1.68s 1.67s -0.5% 1.16s 1.15s 0.25s 0.24s 0.10s 0.13s +20.7% 1.68s 1.74s 1.01x -
synthetic-256-ssr-esm-split 5 1.70s 1.66s -2.4% 1.19s 1.15s 0.24s 0.24s 0.10s 0.10s -0.1% 1.67s 1.70s 1.02x -
synthetic-48-ssr-esm 5 0.89s 0.87s -2.6% 0.63s 0.61s 0.12s 0.12s 0.05s 0.05s -0.7% 0.87s 0.88s 1.03x -

large-355-ssr-esm Plugin Operations

Environment Operation Count Base total Head total Delta Head wall Head max Reports
web route:module 1071 1062.5ms 1087.7ms +2.4% 1087.7ms 18.8ms 6
node route:module 1071 614.2ms 528.7ms -13.9% 528.7ms 10.4ms 6
web route:client-entry 1071 260.6ms 242.6ms -6.9% 242.6ms 5.0ms 6
node manifest:transform 3 70.3ms 90.6ms +28.9% 90.6ms 43.0ms 3
web manifest:stage 6 8.6ms 8.6ms 0.0% 8.6ms 1.9ms 6
web manifest:transform 3 0.3ms 0.3ms 0.0% 0.3ms 0.1ms 3

synthetic-1024-ssr-esm Plugin Operations

Environment Operation Count Base total Head total Delta Head wall Head max Reports
web route:module 3078 1293.5ms 1277.5ms -1.2% 1277.5ms 19.5ms 6
node route:module 3078 552.6ms 562.6ms +1.8% 562.6ms 6.3ms 6
web route:client-entry 3078 365.2ms 398.4ms +9.1% 398.4ms 6.3ms 6
node manifest:transform 3 122.5ms 128.0ms +4.5% 128.0ms 46.9ms 3
node module:client-only-stub 3 46.4ms 138.8ms +199.1% 138.8ms 63.1ms 3
web manifest:stage 6 34.6ms 28.1ms -18.8% 28.1ms 6.6ms 6
web manifest:transform 3 0.3ms 0.3ms 0.0% 0.3ms 0.1ms 3

synthetic-1024-ssr-esm-split Plugin Operations

Environment Operation Count Base total Head total Delta Head wall Head max Reports
web route:module 3078 1210.7ms 1271.2ms +5.0% 1271.2ms 25.8ms 6
node route:module 3078 564.8ms 602.4ms +6.7% 602.4ms 12.3ms 6
web route:client-entry 3078 378.1ms 407.4ms +7.7% 407.4ms 6.2ms 6
node manifest:transform 3 133.4ms 144.0ms +7.9% 144.0ms 62.1ms 3
node module:client-only-stub 3 48.8ms 142.1ms +191.2% 142.1ms 104.5ms 3
web manifest:stage 6 28.2ms 29.7ms +5.3% 29.7ms 7.9ms 6
web manifest:transform 3 0.3ms 0.3ms 0.0% 0.3ms 0.1ms 3

synthetic-256-sourcemaps Plugin Operations

Environment Operation Count Base total Head total Delta Head wall Head max Reports
web route:module 1291 703.3ms 755.7ms +7.5% 755.7ms 12.4ms 11
node route:module 1290 311.5ms 310.0ms -0.5% 310.0ms 8.8ms 10
web route:client-entry 1291 189.9ms 197.1ms +3.8% 197.1ms 5.2ms 11
node manifest:transform 5 68.5ms 66.9ms -2.3% 66.9ms 20.5ms 5
node module:client-only-stub 5 43.1ms 107.8ms +150.1% 107.8ms 54.2ms 5
web manifest:stage 11 11.7ms 11.5ms -1.7% 11.5ms 1.5ms 11
web manifest:transform 5 0.5ms 0.5ms 0.0% 0.5ms 0.1ms 5

synthetic-256-ssr-esm Plugin Operations

Environment Operation Count Base total Head total Delta Head wall Head max Reports
web route:module 1291 687.1ms 695.6ms +1.2% 695.6ms 13.5ms 11
node route:module 1290 279.7ms 272.3ms -2.6% 272.3ms 4.9ms 10
web route:client-entry 1291 200.7ms 190.8ms -4.9% 190.8ms 3.7ms 11
node module:client-only-stub 5 134.0ms 61.4ms -54.2% 61.4ms 33.2ms 5
node manifest:transform 5 78.5ms 63.7ms -18.9% 63.7ms 16.5ms 5
web manifest:stage 11 10.7ms 11.7ms +9.3% 11.7ms 1.5ms 11
web manifest:transform 5 0.4ms 0.5ms +25.0% 0.5ms 0.1ms 5

synthetic-256-ssr-esm-split Plugin Operations

Environment Operation Count Base total Head total Delta Head wall Head max Reports
web route:module 1291 689.1ms 689.4ms +0.0% 689.4ms 9.9ms 11
node route:module 1290 292.5ms 296.9ms +1.5% 296.9ms 5.1ms 10
web route:client-entry 1291 202.8ms 181.0ms -10.7% 181.0ms 3.7ms 11
node manifest:transform 5 82.2ms 88.0ms +7.1% 88.0ms 21.3ms 5
node module:client-only-stub 5 52.4ms 211.3ms +303.2% 211.3ms 145.1ms 5
web manifest:stage 11 10.7ms 11.9ms +11.2% 11.9ms 1.8ms 11
web manifest:transform 5 0.5ms 0.5ms 0.0% 0.5ms 0.1ms 5

synthetic-48-ssr-esm Plugin Operations

Environment Operation Count Base total Head total Delta Head wall Head max Reports
web route:module 250 222.1ms 192.9ms -13.1% 192.9ms 7.9ms 10
node route:module 250 84.1ms 81.8ms -2.7% 81.8ms 1.4ms 10
web route:client-entry 250 58.7ms 49.8ms -15.2% 49.8ms 3.3ms 10
node module:client-only-stub 5 29.6ms 51.1ms +72.6% 51.1ms 16.2ms 5
node manifest:transform 5 29.1ms 25.0ms -14.1% 25.0ms 5.9ms 5
web manifest:stage 10 2.7ms 2.6ms -3.7% 2.6ms 0.4ms 10
web manifest:transform 5 0.5ms 0.5ms 0.0% 0.5ms 0.1ms 5

Synthetic Rsbuild App

Rendered 2 production build benchmarks.

Benchmark Runs Base total Head total Delta Head mean Head p95 Speedup Head RSS p95
complex app 3 111.61s 118.72s +6.4% 118.63s - 0.94x -
complex app 3 83.64s 84.45s +1.0% 84.26s - 0.99x -

Rendered 1 dev benchmark fixture from the embedded complex app.

Benchmark Runs Base total Head total Delta Base ready Head ready Base routes Head routes Base update/HMR Head update/HMR Update delta Head mean Head p95 Speedup Head RSS p95
complex app 3 102.23s 103.60s +1.3% 92.11s 93.44s 3.18s 3.26s 2.45s 2.48s +0.8% 103.58s - 0.99x -

Profile: ci-small+ci-large; mode: dev; iterations: 5; warmup: 1.
The uploaded benchmark artifact includes diagnostics/summary.md and diagnostics/summary.json with runner metadata, per-run timing samples, CPU/RSS samples, and plugin timing hot spots.
Workflow run

@ScriptedAlchemy
ScriptedAlchemy force-pushed the codex/dev-rebuild-optimizations-20260708 branch from 5b459a4 to d09dfb1 Compare July 8, 2026 20:50
@matthewdavis-oai

Copy link
Copy Markdown
Contributor

The current PR description appears stale in two places: the head keeps runtimeChunk: 'single' and does not force splitChunks: false, and the worker/executor result-cache path has been replaced by the dev Rspack loader plus the production API-transform path.

The latest CI comparison is still useful evidence for this work: update/HMR improves from 2.92 s to 2.63 s (-10.1%) across the seven dev fixtures, 1.72 s to 1.55 s (-9.9%) on the large fixture, and 3.37 s to 2.43 s (-27.9%) on the complex synthetic app. Ready time and several standard-fixture totals regress (all-fixture ready 19.60 s to 21.15 s, +7.9%), with route:module attribution increasing substantially in the larger cases. Calling out both results would make the performance/correctness handoff clearer.

The removal of forced dev.writeToDisk: true is present and preserves an explicit user setting, which is the relevant memory-filesystem/HMR fix.

@matthewdavis-oai

matthewdavis-oai commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

One correctness gap in the installed-package development path is worth covering before this lands: route-module-transform-loader calls executeRouteTransformTask({ kind: 'routeModule', ... }) without devHmr, while the API-transform path supplies the computed development-HMR value. route-transform-tasks only appends missing $RefreshReg$ registrations for already-lowered JSX/MDX when that value is enabled. A route can therefore remount and lose state after an update even though the source-test/API path passes.

A focused fix would thread the computed web React-Refresh/HMR-enabled value into the loader rule/options (or derive the same value from loader compiler options), forward it to the task, and add a loader-path regression using already-lowered JSX that asserts $RefreshReg$ plus a state-preserving update. The current RSTEST API-transform fallback and loader tests do not exercise that boundary.

There is also a source-map correctness gap: the new loader is enforce: 'post', so preceding MDX/SWC/other loaders can pass transformed source and an incoming map. The loader accepts only source and returns a newly generated map, replacing rather than composing the incoming mapping. That can point devtools at the wrong original route lines/columns. Accept and compose inputSourceMap (or use a map-aware transform path) and add a two-loader regression that verifies traced original positions, not merely that a map exists.

Finally, the loader's performance-profiler map/timer is process-local. With parallel loader workers, each worker aggregates its own route:module samples, so the reported operation totals are not one compiler/environment-wide aggregate. Either merge reports in a coordinator/compiler hook or explicitly label them per worker and add a multi-worker attribution assertion; the current two-call test covers only a single thread.

One option-semantics caveat for the description/benchmark output: production now always selects the API-transform path and its runner is inline, so parallelRouteTransform: true | number has no production effect even though benchmark output can still display maxWorkers. Document the option as development-only (and mark it ignored in build reporting), or retain a supported parallel build path; a config/action regression would prevent the silent mismatch.

@ScriptedAlchemy
ScriptedAlchemy marked this pull request as ready for review July 28, 2026 01:05

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9e126dc301

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +53 to +57
const parallel = getRouteModuleTransformParallel(parallelRouteTransform);
return {
loader: routeModuleTransformLoaderPath,
options,
...(parallel ? { parallel } : {}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Gate the parallel loader on the declared Rsbuild range

When parallelRouteTransform is enabled explicitly, or automatically for 256+ routes, this emits Rspack's new parallel loader option unconditionally. The package still declares @rsbuild/core: ^2.0.0 as a peer, although this commit needed to upgrade its own toolchain to Rsbuild 2.1.5/Rspack 2.1.3 for this API; consumers on an accepted older peer version will therefore reject the rule during configuration instead of building. Raise the peer minimum or feature-detect this capability and fall back to a non-parallel loader.

Useful? React with 👍 / 👎.

Comment thread src/types.ts Outdated
Comment on lines +60 to +63
* Run route module transforms with Rspack's parallel loader workers.
* Pass `true` to use Rspack's default worker count, a positive integer to set
* the maximum worker count, or `false` to disable.
* @default Automatically enabled for 256+ resolved routes.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Update the public README with the new worker semantics

These public option semantics now delegate true to Rspack's default worker count and treat a number as a maximum, but README.md:87 still promises that development uses zero workers on machines with four or fewer cores and describes a number as the worker count. Users following the published README can consequently enable workers in environments where they expected inline transforms and see unexpected resource usage; update the README alongside this API change.

Useful? React with 👍 / 👎.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The route transformation pipeline now uses a Rspack loader and task runner instead of the previous worker executor. New rule registration handles route queries, route paths, source maps, performance reporting, and configurable parallel workers. Build and development wiring was updated, federation async startup was enabled, benchmark reporting gained worker metadata, and dependencies were upgraded. Tests cover loader behavior, transform rules, integration behavior, federation configuration, performance aggregation, and benchmark summaries.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and matches the main dev rebuild optimization theme of the changeset.
Description check ✅ Passed The description is clearly related to the changeset and summarizes the main implementation goals.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/dev-rebuild-optimizations-20260708

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/federation.ts`:
- Around line 20-27: Update the pluginName check in the federation plugin
detection flow to accept both the legacy “ModuleFederationPlugin” name and the
enhanced Rspack “RspackModuleFederationPlugin” name, while preserving the
existing option extraction. Add coverage for both plugin names, including the
enhanced constructor variant, and verify each enables experiments.asyncStartup.

In `@src/index.ts`:
- Around line 408-412: Update the route transform execution around
parallelRouteTransform and routeTransformRunner so useApiRouteModuleTransforms
mode also honors the configured parallelRouteTransform setting. Route API
transforms through the parallel task runner when enabled, or explicitly enforce
loader-only applicability for the option and preserve that contract consistently
at the additional referenced call site.

In `@tests/dev-runtime.integration.test.ts`:
- Around line 464-466: Update the hasDiskOutput assertion in the relevant
integration test to check build/client/virtual/react-router/browser-manifest.js,
removing the static/js segments so it validates the actual emitted
browser-manifest location.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: db524a5f-4104-4e2b-9bb1-30f181c3abd4

📥 Commits

Reviewing files that changed from the base of the PR and between 94ed3cc and 9e126dc.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (34)
  • benchmarks/synthetic-web-bundler-benchmark/package.json
  • config/package.json
  • examples/client-only/package.json
  • examples/cloudflare/package.json
  • examples/custom-node-server/package.json
  • examples/default-template/package.json
  • examples/epic-stack/package.json
  • examples/federation/epic-stack-remote/package.json
  • examples/federation/epic-stack/package.json
  • examples/prerender/package.json
  • examples/spa-mode/package.json
  • package.json
  • rslib.config.ts
  • scripts/benchmark/results.mts
  • src/build-output-transforms.ts
  • src/dev-background-resources.ts
  • src/federation.ts
  • src/index.ts
  • src/parallel-route-transform-protocol.ts
  • src/parallel-route-transform-worker.ts
  • src/parallel-route-transforms.ts
  • src/performance.ts
  • src/route-module-transform-loader.ts
  • src/route-module-transform-rules.ts
  • src/route-transform-tasks.ts
  • src/types.ts
  • tests/benchmark-results.test.ts
  • tests/build-output-transforms.test.ts
  • tests/dev-runtime.integration.test.ts
  • tests/federation.test.ts
  • tests/index.test.ts
  • tests/parallel-route-transforms.test.ts
  • tests/route-module-transform-loader.test.ts
  • tests/route-module-transform-rules.test.ts
💤 Files with no reviewable changes (4)
  • src/parallel-route-transform-worker.ts
  • src/parallel-route-transform-protocol.ts
  • tests/parallel-route-transforms.test.ts
  • src/parallel-route-transforms.ts

Comment thread src/federation.ts Outdated
Comment on lines +20 to +27
const pluginName = (plugin as ModuleFederationPluginLike).name;
if (pluginName !== 'ModuleFederationPlugin') {
continue;
}

const pluginOptions =
(plugin as ModuleFederationPluginLike)._options ??
(plugin as ModuleFederationPluginLike).options;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate federation.ts =="
fd -a 'federation\.ts$' . || true

echo "== file excerpt =="
if [ -f src/federation.ts ]; then
  nl -ba src/federation.ts | sed -n '1,140p'
fi

echo "== related federation symbols =="
rg -n "ensureFederationAsyncStartup|RspackModuleFederationPlugin|ModuleFederationPlugin|asyncStartup|_options|options" -S . --glob '!node_modules' --glob '!dist' --glob '!build' || true

Repository: rstackjs/rsbuild-plugin-react-router

Length of output: 456


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== file excerpt =="
if [ -f src/federation.ts ]; then
  awk '{printf "%6d\t%s\n", NR, $0}' src/federation.ts | sed -n '1,160p'
fi

echo "== related federation symbols =="
rg -n "ensureFederationAsyncStartup|`@rspack/`|RspackModuleFederationPlugin|ModuleFederationPlugin|asyncStartup|mangleInternalNames|_options|options" -S . --glob '!node_modules' --glob '!dist' --glob '!build' || true

echo "== package hints =="
for f in package.json pnpm-lock.yaml yarn.lock package-lock.json; do
  [ -f "$f" ] && { echo "--- $f ---"; sed -n '1,220p' "$f"; }
done

Repository: rstackjs/rsbuild-plugin-react-router

Length of output: 50392


🌐 Web query:

@module-federation/extended rspack ModuleFederationPlugin construction plugin name RspackModuleFederationPlugin

💡 Result:

In the ecosystem of Rspack and Module Federation, the term RspackModuleFederationPlugin is not a standard export name for the primary Module Federation plugin [1][2]. Instead, you should use the ModuleFederationPlugin class, which is exported by the relevant packages depending on your required feature set [3][4]. For Rspack, there are two primary ways to implement Module Federation: 1. Using the built-in Rspack plugin: If you are using the native Module Federation capabilities built into Rspack, you import the plugin directly from Rspack's container namespace [1][4]: import { rspack } from '@rspack/core'; // Inside your plugins array new rspack.container.ModuleFederationPlugin({ // options }) 2. Using the @module-federation/enhanced package: For advanced features (often referred to as Module Federation v2.0), such as dynamic TypeScript type hints, improved runtime plugins, and better tree shaking for shared dependencies, you use the @module-federation/enhanced package [2][3][4]. The plugin is imported specifically for Rspack from this package [3][4]: import { ModuleFederationPlugin } from '@module-federation/enhanced/rspack'; // Inside your plugins array new ModuleFederationPlugin({ // options }) Regarding the name of the plugin class, the internal identifier (often used for logging or duplicate registration checks) is defined as PLUGIN_NAME within the source code of the @module-federation/core repository, but this is not the class name you use in your configuration [5]. Always refer to the class as ModuleFederationPlugin as shown above [3][4].

Citations:


🌐 Web query:

@module-federation/enhanced rspack ModuleFederationPlugin construction plugin name RspackModuleFederationPlugin

💡 Result:

When using Module Federation with Rspack, particularly the modern implementation provided by the @module-federation/enhanced package, the relevant plugin is typically imported from @module-federation/enhanced/rspack [1]. Regarding your specific query about the construction of the plugin and the name RspackModuleFederationPlugin: 1. Plugin Class and Name: In the source code of the @module-federation/core repository, specifically within the package that provides Rspack integration, the class exported as ModuleFederationPlugin explicitly defines its internal name property as RspackModuleFederationPlugin [2]. This internal name is used by the plugin for logging, hook identification, and internal referencing during the build process [2]. 2. Usage: Users generally do not construct or reference RspackModuleFederationPlugin directly by that name in their configuration files. Instead, they import the ModuleFederationPlugin class from the @module-federation/enhanced/rspack entry point [1][3]. Example of standard construction: import { ModuleFederationPlugin } from '@module-federation/enhanced/rspack'; export default { plugins: [ new ModuleFederationPlugin({ // options }), ], }; 3. Context: While the class instance is named internally as RspackModuleFederationPlugin, it serves as the interface for Module Federation v2.0 (and 1.5 features) within Rspack, providing capabilities such as runtime plugins and improved tree shaking for shared dependencies [1]. Rspack also provides a built-in ModuleFederationPlugin (available via rspack.container.ModuleFederationPlugin) which implements Module Federation 1.5, and a legacy ModuleFederationPluginV1 [4][1][5].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== tests/federation.test.ts =="
if [ -f tests/federation.test.ts ]; then
  awk '{printf "%6d\t%s\n", NR, $0}' tests/federation.test.ts
fi

echo "== fetch `@module-federation/enhanced` rspack source for Plugin name =="
node - <<'JS'
const https = require('https');
const url = 'https://unpkg.com/@module-federation/enhanced@2.5.1/dist/rspack/esm/ModuleFederationPlugin.js';
https.get(url, (res) => {
  let data = '';
  res.on('data', chunk => data += chunk);
  res.on('end', () => {
    const lines = data.split(/\r?\n/).filter(line =>
      /class\s+ModuleFederationPlugin|static\s*#(?<name>|NAME|PLUGIN_NAME)|(?<name>name|constructor\.name|PLUGIN_NAME)\s*:|constructor?/
        .test(line)
    );
    for (let i = 0; i < Math.min(lines.length, 120); i++) console.log(`${i+1} ${lines[i]}`);
  });
}).on('error', e => {
  console.error(e.message);
  process.exit(1);
});
JS

echo "== fetch `@module-federation/rspack` source for legacy plugin constructor/name =="
node - <<'JS'
const https = require('https');
const url = 'https://unpkg.com/@module-federation/rspack@0.21.6/dist/plugin.esm.mjs';
https.get(url, (res) => {
  let data = '';
  res.on('data', chunk => data += chunk);
  res.on('end', () => {
    const lines = data.split(/\r?\n/).filter(line =>
      /class\s+ModuleFederationPlugin|Name|name|constructor|PLUGIN_NAME|rspack/i
        .test(line)
    );
    for (let i = 0; i < Math.min(lines.length, 120); i++) console.log(`${i+1} ${lines[i]}`);
  });
}).on('error', e => {
  console.error(e.message);
  process.exit(1);
});
JS

echo "== deterministic probe of current selector =="
node - <<'JS'
function selects(plugin) {
  const pluginName = plugin.name;
  return pluginName === 'ModuleFederationPlugin';
}
const legacy = {
  name: 'ModuleFederationPlugin',
  options: { experiments: { asyncStartup: false } }
};
const enhanced = {
  name: 'RspackModuleFederationPlugin',
  options: { experiments: { asyncStartup: false } }
};
console.log(JSON.stringify({ legacySelected: selects(legacy), enhancedSelected: selects(enhanced), enhancedName: enhanced.name, enhancedConstructorName: enhanced.constructor?.name }));
JS

Repository: rstackjs/rsbuild-plugin-react-router

Length of output: 2901


Match Rspack federation plugins by their actual plugin name.

@module-federation/enhanced/rspack’s ModuleFederationPlugin sets plugin.name = 'RspackModuleFederationPlugin', so this exact check keeps the enhanced examples from enabling experiments.asyncStartup. Add coverage for both the legacy ModuleFederationPlugin and the enhanced plugin name/constructor variant.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/federation.ts` around lines 20 - 27, Update the pluginName check in the
federation plugin detection flow to accept both the legacy
“ModuleFederationPlugin” name and the enhanced Rspack
“RspackModuleFederationPlugin” name, while preserving the existing option
extraction. Add coverage for both plugin names, including the enhanced
constructor variant, and verify each enables experiments.asyncStartup.

Comment thread src/index.ts Outdated
Comment thread tests/dev-runtime.integration.test.ts Outdated
Comment on lines +464 to +466
harness.hasDiskOutput(
'build/client/static/js/virtual/react-router/browser-manifest.js'
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Check the emitted browser-manifest path.

[name].js emits this entry at build/client/virtual/react-router/browser-manifest.js; the extra static/js makes this assertion pass even if the manifest is written to disk.

Proposed fix
-      'build/client/static/js/virtual/react-router/browser-manifest.js'
+      'build/client/virtual/react-router/browser-manifest.js'
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
harness.hasDiskOutput(
'build/client/static/js/virtual/react-router/browser-manifest.js'
)
harness.hasDiskOutput(
'build/client/virtual/react-router/browser-manifest.js'
)
🧰 Tools
🪛 ast-grep (0.44.1)

[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(detect-child-process-typescript)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/dev-runtime.integration.test.ts` around lines 464 - 466, Update the
hasDiskOutput assertion in the relevant integration test to check
build/client/virtual/react-router/browser-manifest.js, removing the static/js
segments so it validates the actual emitted browser-manifest location.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/route-module-transform-rules.ts (1)

24-42: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Do not silently ignore parallelRouteTransform on the API path.

This helper only configures the loader rules, but src/index.ts bypasses those rules whenever shouldUseRouteModuleTransformApi() is true. As a result, the option has no effect on the inline/API path, including production. Preserve parallelism there or explicitly document and validate this option as development-only.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/route-module-transform-rules.ts` around lines 24 - 42, The
parallelRouteTransform option is ignored when shouldUseRouteModuleTransformApi()
selects the inline/API path. Update the API-path implementation in the
surrounding route-module transform flow to preserve the configured boolean or
worker-count parallelism, including production; otherwise explicitly reject
non-default values with validation and document the development-only constraint.
🧹 Nitpick comments (1)
tests/route-module-transform-rules.test.ts (1)

48-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover both positive API-selection branches.

This test only proves the negative case. Add cases for existsSync === true and RSTEST === 'true'; otherwise regressions in the loader/API dispatch can go undetected.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/route-module-transform-rules.test.ts` around lines 48 - 63, Expand the
route-module transform API tests around shouldUseRouteModuleTransformApi to
cover both positive selection branches: mock fs.existsSync to return true with
RSTEST unset, and set RSTEST to 'true' with existsSync false. Assert the
function returns true in each case, while preserving environment restoration and
spy cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/route-module-transform-rules.ts`:
- Around line 24-42: The parallelRouteTransform option is ignored when
shouldUseRouteModuleTransformApi() selects the inline/API path. Update the
API-path implementation in the surrounding route-module transform flow to
preserve the configured boolean or worker-count parallelism, including
production; otherwise explicitly reject non-default values with validation and
document the development-only constraint.

---

Nitpick comments:
In `@tests/route-module-transform-rules.test.ts`:
- Around line 48-63: Expand the route-module transform API tests around
shouldUseRouteModuleTransformApi to cover both positive selection branches: mock
fs.existsSync to return true with RSTEST unset, and set RSTEST to 'true' with
existsSync false. Assert the function returns true in each case, while
preserving environment restoration and spy cleanup.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e8e26ad3-a392-47a5-97aa-555a5ef8b41b

📥 Commits

Reviewing files that changed from the base of the PR and between 9e126dc and 152af59.

📒 Files selected for processing (11)
  • scripts/benchmark/results.mts
  • src/build-output-transforms.ts
  • src/federation.ts
  • src/index.ts
  • src/route-module-transform-loader.ts
  • src/route-module-transform-rules.ts
  • tests/benchmark-results.test.ts
  • tests/build-output-transforms.test.ts
  • tests/dev-runtime.integration.test.ts
  • tests/route-module-transform-loader.test.ts
  • tests/route-module-transform-rules.test.ts
💤 Files with no reviewable changes (1)
  • tests/dev-runtime.integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
  • tests/benchmark-results.test.ts
  • scripts/benchmark/results.mts
  • tests/route-module-transform-loader.test.ts
  • src/route-module-transform-loader.ts
  • src/build-output-transforms.ts
  • src/index.ts
  • tests/build-output-transforms.test.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/types.ts`:
- Around line 60-63: Document that parallelRouteTransform is development-only
because production uses inline transforms and enabling this option there has no
effect. Update the option description in src/types.ts (lines 60-63) and mirror
the same semantics in the README.md option table (lines 80-89); do not change
production transform behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 78e96e5a-08e5-4786-b7c3-440466bbf9c8

📥 Commits

Reviewing files that changed from the base of the PR and between 152af59 and 01f52b1.

📒 Files selected for processing (7)
  • README.md
  • src/index.ts
  • src/route-module-transform-rules.ts
  • src/types.ts
  • tests/build-output-transforms.test.ts
  • tests/dev-runtime.integration.test.ts
  • tests/route-module-transform-rules.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/index.ts
  • tests/build-output-transforms.test.ts

Comment thread src/types.ts
Comment on lines +60 to +63
* Run route module transforms with Rspack's parallel loader workers.
* Pass `true` to use Rspack's default worker count, a positive integer to set
* the maximum worker count, or `false` to disable.
* @default false

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Document the production no-op for parallelRouteTransform. Production currently uses inline transforms, so this option only affects development; otherwise users can enable it in production and receive no effect.

  • src/types.ts#L60-L63: state that the option is development-only, or wire production through the parallel loader.
  • README.md#L80-L89: mirror the same development-only semantics in the option table.
📍 Affects 2 files
  • src/types.ts#L60-L63 (this comment)
  • README.md#L80-L89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/types.ts` around lines 60 - 63, Document that parallelRouteTransform is
development-only because production uses inline transforms and enabling this
option there has no effect. Update the option description in src/types.ts (lines
60-63) and mirror the same semantics in the README.md option table (lines
80-89); do not change production transform behavior.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.changeset/calm-routes-transform.md:
- Line 5: Update the changeset text to clarify that parallel route transforms
are development-only and that production continues using the inline
API-transform path; do not describe parallelRouteTransform as affecting
production unless production support is added.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e98fe574-1b8a-44f3-acaa-3153f809db7b

📥 Commits

Reviewing files that changed from the base of the PR and between 01f52b1 and 29912bf.

📒 Files selected for processing (1)
  • .changeset/calm-routes-transform.md

'rsbuild-plugin-react-router': minor
---

Keep route transforms inline by default and expose Rspack parallel loader workers as an explicit opt-in.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Document that parallel transforms are development-only.

Production currently uses the inline API-transform path, so parallelRouteTransform has no production effect. Please clarify this in the changeset, or add production support before describing the option without that limitation.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.changeset/calm-routes-transform.md at line 5, Update the changeset text to
clarify that parallel route transforms are development-only and that production
continues using the inline API-transform path; do not describe
parallelRouteTransform as affecting production unless production support is
added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants